Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow finetuning ESM2 with [un]frozen encoder #620

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

farhadrgh
Copy link
Collaborator

@farhadrgh farhadrgh commented Jan 17, 2025

Description

The overlap_grad_reduce=True causes communication issue in gradient synchronization step when the encoder parameters are not frozen.

AssertionError: Communication call has not been issued for this bucket (79/84 params have grad available)

This PR changes the default for overlap_grad_reduce and exposes config.encoder_frozen to optionally [un]freeze the encoder parameters.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactor
  • Documentation update
  • Other (please describe):

CI Pipeline Configuration

Configure CI behavior by applying the relevant labels:

Note

By default, the notebooks validation tests are skipped unless explicitly enabled.

Usage

TODO: Add code snippet

Pre-submit Checklist

  • I have tested these changes locally
  • I have updated the documentation accordingly
  • I have added/updated tests as needed
  • All existing tests pass successfully

Signed-off-by: Farhad Ramezanghorbani <[email protected]>
Signed-off-by: Farhad Ramezanghorbani <[email protected]>
Signed-off-by: Farhad Ramezanghorbani <[email protected]>
Signed-off-by: Farhad Ramezanghorbani <[email protected]>
@farhadrgh farhadrgh marked this pull request as ready for review January 17, 2025 19:23
@farhadrgh farhadrgh requested a review from sichu2023 January 17, 2025 19:23
@farhadrgh farhadrgh enabled auto-merge January 17, 2025 19:24
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.63%. Comparing base (eb89056) to head (af1619e).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #620   +/-   ##
=======================================
  Coverage   86.63%   86.63%           
=======================================
  Files         116      116           
  Lines        6966     6967    +1     
=======================================
+ Hits         6035     6036    +1     
  Misses        931      931           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@sichu2023 sichu2023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@farhadrgh farhadrgh added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit a2fd916 Jan 22, 2025
8 checks passed
@farhadrgh farhadrgh deleted the farhadr/ft_encoder_frozen branch January 22, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants